Store result cache paths relative to the install location - #6190
Store result cache paths relative to the install location#6190SanderMuller wants to merge 8 commits into
Conversation
…ggle The result cache stores absolute paths in its meta, keys and stored objects, and compares metadata with a strict whole-array match, so a changed absolute prefix (a fresh CI checkout dir, a git worktree) throws the whole cache away even when the relative layout is identical. Add a bleeding-edge featureToggle, relativePathResultCache, that stores the paths relative to the phpstan install (%rootDir%) and re-absolutizes them against the current install on load. Only paths reachable from the anchor become relative; the rest stay absolute, following ccache's rule. Error gains relativizePaths()/absolutizePaths(), building on its existing immutable changeFilePath() pattern, and a new ResultCachePathTransformer handles the rest of the cache structure at the save/restore boundary. The toggle state is folded into the cache meta and CACHE_VERSION is bumped so flipping it or upgrading migrates with one cold run. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Warms the cache in one checkout, creates a git worktree at a different absolute path with its own phpstan install, carries the warm cache over, and asserts it is reused with 0 files reanalysed. Proves the relative paths re-absolutize against the worktree, the scenario the toggle targets. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
PHPStan's -vv progress, including the "Result cache restored" line, is written to stderr. The assertion captured stdout only, so it missed the message and failed even though the cache was reused. Redirect stderr into the captured output. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
I don't think this has to be behind bleeding edge. It's not a BC break, it should work the same way as before. It's a good point that some people might be doing something to the structure of the current result cache format to achieve similar things this PR aims to achieve, but that's out of BC surface anyway, and the end result should be that they should have to be able delete their tooling altogether, if we do a good job. As a bonus, more E2E result cache tests will test the new path. |
Per review: this is not a BC break. For a project analysed on the same machine the relativized paths re-absolutize to the exact same absolute paths, so behaviour is unchanged; the only difference is that a moved project (a CI checkout dir, a git worktree) now reuses the cache instead of discarding it. Drop the featureToggle and relativize/absolutize unconditionally. The CACHE_VERSION bump migrates old caches with one cold run, and every result cache e2e now exercises the new path. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Done, made it the default and dropped the featureToggle. Since the on-disk format changes I bumped |
| cp -al ../../vendor "$WORKTREE/vendor" | ||
| cp -R tmp "$WORKTREE/e2e/result-cache-relative-path/tmp" | ||
| rm -rf "$WORKTREE/e2e/result-cache-relative-path/tmp/cache" |
There was a problem hiding this comment.
did I understand it right, that we need this copying because this PR is not going to implement the whole feature in a single shot?
There was a problem hiding this comment.
Yes, exactly. The copy stands in for cache discovery, PHPStan locating the main checkout's warm cache from a fresh worktree, which is the harder part and a deliberate follow-up, not this PR. This PR makes the cache content portable so it can be reused once it is present; getting it present in a bare worktree is the separate step. I noted it in the code comment and the PR description.
Drop the redundant "relative path is present" assertion (the explicit "absolute path is absent" check on the next line already proves the path was relativized), fix a stale "toggle on" comment, and after the git worktree run remove the worktree and re-run in the original checkout to confirm it still reuses its own cache. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
staabm
left a comment
There was a problem hiding this comment.
I think it looks good. lets see what @ondrejmirtes thinks about it
|
(just see CI is failling now) |
Looking into it. Yesterday GitHub had an outage so couldn't properly follow-up on the CI |
…rage The transformer's behaviour is covered end to end by the result cache e2e tests (relative storage, warm reuse, and reuse across a git worktree at a different absolute path). The unit test additionally hardcoded POSIX absolute paths, which the Windows Tests matrix cannot satisfy since path handling there is drive-letter based. Drop it, matching the project's convention of testing result cache behaviour through e2e fixtures. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Would be nice if you ran it locally and grepped the new result cache whether there are still some absolute paths present or not. Also the cache should be portable between Windows and Linux so the directory separators should always be /. PHPStan has FileHelper class for normalization. |
The relative path helper already emits '/'-separated paths for anything reachable from the anchor, but returns a path with no shared prefix unchanged, which on Windows keeps backslashes. Normalise the stored paths to '/' so a cache written on Windows is usable on Linux and vice versa. On load the paths are absolutized back to the OS-native separator that FileFinder uses for analysed-file keys. Namespace separators in class names (FQCNs) are untouched, only path values are normalised. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Did both. I ran self-analysis and grepped the generated result cache for the absolute project prefix: 0 occurrences. Analysed files, the dependency graph, and the meta (including On separators, stored paths now always use |
projectConfig is stored as a relative Neon string and is never absolutized on load; the metadata comparison relativizes the current config instead. So absolutizeProjectConfig() had no caller once the unit test was removed and self-analysis flagged it. Inline the remaining relativize-only logic into relativizeProjectConfig() and drop the now single-use helper. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
just noticed, that this PR has another nice side effect: result-cache size shrinks by ~5% in comparison to 2.2.x on my machine running on phpstan-src |
|
Nice, that makes sense: every stored path drops the absolute prefix (on your checkout the It also scales with how deep the checkout lives: a CI path like |
|
|
Description updated. On CI: the failures were a real one from me. Removing the unit test earlier orphaned a The only red left is |
Makes the result cache portable across a change of the project's absolute path prefix (a fresh CI checkout dir, a git worktree). Today the cache stores absolute paths everywhere (meta, storage keys, stored objects) and compares metadata with a strict whole-array match, so a moved project with an identical relative layout throws the whole cache away.
Approach
%rootDir%, the phar's directory, or the source checkout underbin/phpstan). This is the anchor @ondrejmirtes landed on: a Composer-installed phar sits at a fixed offset inside the project (vendor/phpstan/phpstan), so the path from the anchor to the analysed code is stable across checkouts and worktrees.Errorrelativizes/absolutizes its own paths (per @ondrejmirtes' note), building on its existing immutablechangeFilePath()pattern. A newResultCachePathTransformerhandles the rest of the cache structure (meta, dependencies, the compoundlinesToIgnorekeys, projectConfig) usingParentDirectoryRelativePathHelperto relativize andFileHelper::absolutizePath()to invert. The transform lives at the I/O boundary:save()relativizes before writing,restore()absolutizes right afterrequire; the rest of the manager keeps working in absolute paths./, so a cache written on Windows is usable on Linux and vice versa; on load they are absolutized back to the OS-native separatorFileFinderuses for the analysed-file keys. Namespace separators in class names are left untouched.CACHE_VERSIONis bumped so old caches migrate with one cold run, and every result cache e2e now exercises the new path.Covered surface
Storage: errors, locallyIgnoredErrors, linesToIgnore/unmatchedLineIgnores (including the compound
path (in context of class X)keys), collectedData, dependencies, packageDependencies, exportedNodes, projectExtensionFiles. Meta: analysedPaths, scannedFiles, composerLocks, composerInstalled (including nestedinstall_path), executedFilesHashes, stubFiles, and projectConfigpaths/tmpDir.Verified by grepping a real self-analysis cache: 0 absolute project-prefix paths remain.
Tests
e2e/result-cache-relative-path: a cold run stores the analysed file relative to the anchor (asserting the absolute checkout path is absent), and a warm run reuses the cache with 0 files reanalysed.git worktreeat a different absolute path with its own vendor, carries the warm cache over, and asserts it is reused there with 0 files reanalysed; then removes the worktree and re-runs in the original checkout to confirm that is still reused.metadata do not match: projectConfig, analysedPaths, executedFilesHashesand re-analyses everything.Open question: the anchor
%rootDir%is portable when the phar moves with the project (Composer install, worktree, CI checkout). A globally-installedphpstan.pharoutside the project tree does not share a moving prefix, so its relative offsets would not survive a move.%currentWorkingDirectory%moves with the project in those same scenarios and matches the issue's wording, at the cost of not covering PHPStan's own stub and config paths. The anchor is isolated behindgetPathTransformer()plus the injected%rootDir%, so switching is a one-line change if preferred.Follow-ups (not in this PR)
excludePaths(stored asOptionalPathobjects) is not relativized yet, so a project relying on optional excludePaths still invalidates on a move.ResultCacheMetaExtensionhashes and collector payloads that embed absolute paths cannot be seen into by a core transform; the Symfony container extension already foldskernel.project_dirinto its meta hash and would need its own handling.normalizePath()is lexical and does not resolve symlinks).git worktreewhose tmpDir has no cache would need PHPStan to discover the main checkout's cache. This PR makes the cache content portable; that discovery is a separate step.Closes phpstan/phpstan#8599